Adding initial version of audit log v2 html pattern library #180

jwrosewell · 2022-06-06T13:35:51Z

Added new self contained directory named ./paf-mvp-pattern-library-audit for V2 audit log HTML pattern library in Storybook.
Minor change to audit UI model.

…group

… into feature/audit-log-v2-pattern-library

… additions.

… into feature/audit-log-v2-pattern-library

…can now be version controlled with each other. The CMP figma can also be added into a sibling folder called "CMP".

OlivierChirouze · 2022-06-13T08:35:28Z

Why is there an empty paf-mvp-pattern-library-audit/javascript/ok-ui.js file? Shouldn't it be removed?

OlivierChirouze

A couple of issues that prevent me from approving:

disturbing figma directory
usage of Javascript instead of Typescript
empty ok-ui.js file

OlivierChirouze · 2022-06-13T08:29:13Z

paf-mvp-figma/audit/README.md

+
+## About the authors
+
+[Dot Creative](https://dotcreative.hu) is a Digital Agency. Our main profile is


I'm confused about this whole directory: what is its purpose? why a documentation directory in this implementation repo? I'm not sure why there is such a need for such README, but if there is, I think it should be in (https://github.com/prebid/addressability-framework)

The .fig file is the one that contains all the design work which is the input into the pattern library. For those working on the design this file is important to be able to apply updates and understand the work. It is not relevant to developers working on code, but is relevant for the overall project. It could be moved to addressability-framework if that is the better repository. Please let me know once you've considered the .fig file.

Yes, I think this would be better placed in the other repository, because it is focused on specs and design, while this one is focused on implementation.
WDYT?

paf-mvp-figma/audit/README.md

paf-mvp-pattern-library-audit/stories/Introduction.stories.mdx

OlivierChirouze · 2022-06-13T08:38:56Z

paf-mvp-pattern-library-audit/stories/components/AdvertImage.js

+import Details from './Details';
+import { BlockedImage } from './Icons';
+
+const Image = (args = {}) => {


Why not using Typescript? (https://storybook.js.org/docs/react/configure/typescript)

The pattern library is produced to be final implementation agnostic. These will be moved to typescript when the HTML and CSS is transferred for the reference implementation. Other optimisations will also be applied to reduce duplications, etc. The advantage of the approach is that someone could take this HTML and JS and deploy it in PHP (or another language) without any issues.

Thanks.
I'm sorry, I don't understand this sentence:

These will be moved to typescript when the HTML and CSS is transferred for the reference implementation

Could you please develop? What do you mean by "transferred for the reference implementation"?

My understanding is that it's pretty common to define components and stories in typescript, isn't it?
https://storybook.js.org/docs/react/get-started/whats-a-story

paf-mvp-pattern-library-audit/stories/components/AdvertImage.js

paf-mvp-pattern-library-audit/stories/components/AdvertStatus.js

paf-mvp-pattern-library-audit/stories/components/Icons.js

… pattern library.

…ign works.

… into feature/audit-log-v2-pattern-library

paf-mvp-figma/audit/README.md

OlivierChirouze · 2022-06-15T15:25:43Z

paf-mvp-pattern-library-audit/stories/components/AdvertImage.js

+import Details from './Details';
+import { BlockedImage } from './Icons';
+
+const Image = (args = {}) => {


Thanks.
I'm sorry, I don't understand this sentence:

These will be moved to typescript when the HTML and CSS is transferred for the reference implementation

Could you please develop? What do you mean by "transferred for the reference implementation"?

OlivierChirouze · 2022-06-15T15:26:52Z

paf-mvp-figma/audit/README.md

+
+## About the authors
+
+[Dot Creative](https://dotcreative.hu) is a Digital Agency. Our main profile is


Yes, I think this would be better placed in the other repository, because it is focused on specs and design, while this one is focused on implementation.
WDYT?

OlivierChirouze

Still waiting for more details on the JS versus TS.

…from the MVP.

jwrosewell · 2022-06-15T15:38:06Z

PR for the UI design element is now here.

jwrosewell · 2022-06-15T15:41:13Z

Still waiting for more details on the JS versus TS.

The HTML Pattern Library from Storybook is designed to be implemented in any environment and might not include Typescript. For example a PHP server serving static HTML and JS only. The Typescript conversion for this concrete implementation is done, if warranted, when the pattern library is used in the Typescript module. See the use of Popper in the CMP module.

OlivierChirouze · 2022-06-15T16:30:20Z

Let me rephrase with some more basic English 😅
You want these components to be reusable in a different context, for example on a static website that only uses plain JS. For this reason, you want to stick to Vanilla JS for defining the components and stories, instead of TS.

If that's a correct interpretation, then I get your point, although:

I believe at least stories could be written in typescript as they are not part of the production code
reading docs on Storybook, it seems it is pretty standard to write components in tsx. Wouldn't be easy to have a build script that generates js files, if needed for a "vanilla JS" project?

Finally, this is nit picking but I think the components should be stored in a different directory than the stories, or at least the parent one should not be called "stories" as this can be misleading (at least I was confused).

jwrosewell · 2022-06-15T16:40:37Z

As with many things there are different ways of doing the same thing. In this case we've used JS in the Storybook pattern libraries. Happy for others to refactor to TS in the future if they wish.

OlivierChirouze · 2022-06-15T19:06:52Z

paf-mvp-pattern-library-audit/package.json

+    "core-js": "^3.21.0",
+    "css-loader": "^5.2.7",
+    "sass": "^1.49.9",
+    "sass-loader": "^10.1.1",


I had to install @mdx-js/react to be able to start Storybook.
Isn't missing from the dependencies?

Also, NIT but the package-lock.json file seems to have been generated with a version of node that is too old: it should be in version 2 for node 17 (which is the version used on the repo, see .nvmrc)

Re: Package - We've not had to install additional packages. I guess there is no harm in adding it but I can't recreate the issue.
Re: Version - We tend not to use the odd versions of Node because they aren’t the stable releases. We’ve opted for v16 as although v18 was recently released it doesn’t appear to be immediately compatible with our version of Storybook.

(cherry picked from commit 6d34b18)

jwrosewell · 2022-06-16T11:12:43Z

Re: "Transfer to the reference implementation"
I meant that the pattern library provides the CSS and HTML which is then incorporated by the developer into the reference implementation of the module under the ./src/HTML and ./src/CSS folders. Sometimes parts need to be modified or changed to support the locale language object, the visibility of items, or removing duplication. Once they are there the rollup bundles them as template strings into the final assembly. I can cover via a conversation if that will help.

OlivierChirouze

Approving after our sync last week.

Please note that we consider the migration of Storybook components and stories from Javascript to Typescript is a must-have, but should be done in another PR.

johnnorris and others added 8 commits May 23, 2022 14:19

Pattern library (Audit log v2) Initial commit

09cdb69

Pattern library (Audit log v2) improvements

dff79c4

Pattern library (Audit log v2) Add a landscape layout for the advert …

a0daf00

…group

Pattern library (Audit log v2) Add new Website Advert component

e20385f

Added VS Code support for the audit pattern library.

fdf0400

Merge branch 'main' of https://github.com/prebid/paf-mvp-implementation…

7f6601a

… into feature/audit-log-v2-pattern-library

Improved implementation of the model ahead of merging in more complex…

dcaa196

… additions.

Merge branch 'main' of https://github.com/prebid/paf-mvp-implementation…

cc90587

… into feature/audit-log-v2-pattern-library

jwrosewell requested a review from a team as a code owner June 6, 2022 13:35

Added the figma used as the input to the HTML pattern library. These …

f5571cf

…can now be version controlled with each other. The CMP figma can also be added into a sibling folder called "CMP".

OlivierChirouze requested changes Jun 13, 2022

View reviewed changes

jwrosewell added 2 commits June 14, 2022 14:33

Updated to remove empty javascript output which is not needed for the…

c932a41

… pattern library.

Updated the figma readme and images to better demonstrate how the des…

56f8155

…ign works.

jwrosewell mentioned this pull request Jun 14, 2022

Add a CONTRIBUTORS.md to list contributors and how to contact them #186

Closed

Merge branch 'main' of https://github.com/prebid/paf-mvp-implementation…

6f170f2

… into feature/audit-log-v2-pattern-library

OlivierChirouze approved these changes Jun 15, 2022

View reviewed changes

OlivierChirouze requested changes Jun 15, 2022

View reviewed changes

Moved the UI design to the addressability-framework repo and removed …

bde51d7

…from the MVP.

Added CONTRIBUTORS.md to list organisations that worked on the project.

d62faa5

jwrosewell mentioned this pull request Jun 15, 2022

Adds async identity collection to the audit UI #191

Open

OlivierChirouze reviewed Jun 15, 2022

View reviewed changes

Pattern library (Audit log v2) fix broken link in documentation

004f9da

(cherry picked from commit 6d34b18)

OlivierChirouze approved these changes Jun 20, 2022

View reviewed changes

OlivierChirouze merged commit 20e5ba7 into OneKey-Network:main Jun 20, 2022


		## About the authors

		[Dot Creative](https://dotcreative.hu) is a Digital Agency. Our main profile is

Adding initial version of audit log v2 html pattern library #180

Adding initial version of audit log v2 html pattern library #180

Uh oh!

Conversation

jwrosewell commented Jun 6, 2022

Uh oh!

OlivierChirouze commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlivierChirouze left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlivierChirouze left a comment

Choose a reason for hiding this comment

Uh oh!

jwrosewell commented Jun 15, 2022

Uh oh!

jwrosewell commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlivierChirouze commented Jun 15, 2022

Uh oh!

jwrosewell commented Jun 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwrosewell commented Jun 16, 2022

Uh oh!

OlivierChirouze left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

OlivierChirouze commented Jun 13, 2022 •

edited

Loading

jwrosewell commented Jun 15, 2022 •

edited

Loading